Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clamp MSS to the MTU of the source only #68

Open
wants to merge 1 commit into
base: 1.1
Choose a base branch
from

Conversation

dechamps
Copy link
Contributor

Currently, tinc clamps MSS to the minimum of the MTU of both the source and the destination of the packet containing the MSS option. This is slightly suboptimal, because both the MTU and the MSS can differ based on the direction of travel.

For example, if the MTU from A to B is 1500, and the MTU from B to A is 1000, then tinc will clamp MSS in both directions to 960, despite the fact that A to B could use a 1460 MSS.

RFC 793 defines the MSS option as follows:

If this option is present, then it communicates the maximum receive segment size at the TCP which sends this segment.

It follows that the MSS should be clamped to the MTU to the sender of the MSS option. It should not be clamped to the MTU to the destination of the MSS option.

Currently, tinc clamps MSS to the minimum of the MTU of both the source
and the destination of the packet containing the MSS option. This is
slightly suboptimal, because both the MTU and the MSS can differ based
on the direction of travel.

For example, if the MTU from A to B is 1500, and the MTU from B to A is
1000, then tinc will clamp MSS in *both* directions to 960, despite
the fact that A to B could use a 1460 MSS.

RFC 793 defines the MSS option as follows:

  If this option is present, then it communicates the maximum
  receive segment size at the TCP which sends this segment.

It follows that the MSS should be clamped to the MTU *to* the *sender*
of the MSS option. It should not be clamped to the MTU to the
destination of the MSS option.
@gsliepen
Copy link
Owner

On Sat, Mar 14, 2015 at 08:33:39AM -0700, Etienne Dechamps wrote:

Currently, tinc clamps MSS to the minimum of the MTU of both the source and the destination of the packet containing the MSS option. This is slightly suboptimal, because both the MTU and the MSS can differ based on the direction of travel.

For example, if the MTU from A to B is 1500, and the MTU from B to A is 1000, then tinc will clamp MSS in both directions to 960, despite the fact that A to B could use a 1460 MSS.

I put in this code explicitly to help in a real situation where this was
necessary. As long as VPN traffic stays inside the VPN, this wouldn't be
necessary, but if someone has an exit node, and some firewalls between
the exit node and the Internet host that is the non-VPN endpoint of the
traffic blocks ICMP packet too big messages coming from the exit node
(because the tunnel MTU is likely lower than 1500), MSS clamping is the
only way to make sure TCP connections don't use too big packets.

                                       |

VPN host A ================ VPN host B ----|---- Internet host C
|
ICMP blocking firewall

Met vriendelijke groet / with kind regards,
Guus Sliepen [email protected]

@dechamps
Copy link
Contributor Author

I'm confused. Maybe I'm misinterpreting your message, but you seem to imply that I'm removing MSS Clamping entirely. I'm not. In this commit, MSS Clamping is still taking place in both directions. The only difference is the value the MSS is clamped to. It's just a slight optimization that allows each node to use the optimal MSS for each direction of travel, instead of having MSS for both directions be set to MIN(A → B, B → A), which is slightly less optimal.

I'm not sure I understand your objection. Could you please explain in more detail how exactly my patch would result in problems with the network configuration you described?

@gsliepen
Copy link
Owner

On Sun, Apr 12, 2015 at 08:11:42AM -0700, Etienne Dechamps wrote:

I'm confused. Maybe I'm misinterpreting your message, but you seem to imply that I'm removing MSS Clamping entirely. I'm not. In this commit, MSS Clamping is still taking place in both directions. The only difference is the value the MSS is clamped to. It's just a slight optimization that allows each node to use the optimal MSS for each direction of travel, instead of having MSS for both directions be set to MIN(A → B, B → A), which is slightly less optimal.

I'm not sure I understand your objection. Could you please explain in more detail how exactly my patch would result in problems with the network configuration you described?

Now you confused me as well. I'll try to see if I have the exact details
of the problem in my email archive somewhere.

Apart from that, do you know of a situation where the PMTU of A->B is
different from B->A? Not counting cases where tinc misprobed. If it's
rare to non-existant, I'd rather keep clamp_mss() as it is.

Met vriendelijke groet / with kind regards,
Guus Sliepen [email protected]

@dechamps
Copy link
Contributor Author

PMTU(A → B) ≠ PMTU(B → A) is technically possible and would still be compliant with Internet standards. It can technically happen with asymmetrical routing paths and other weird network configurations.

To be honest, I'm not encountering that scenario myself and I think this is a rare case. Furthermore, this is an optimization, not a fix. That said, it makes the function slightly simpler and more "correct" with respect to how MSS clamping is supposed to work, so the patch is still useful. I don't feel strongly about any of this though.

@mark-stopka
Copy link

@dechamps, are you going to resolve the conflicts if it's still helpful?

@fangfufu fangfufu added 1.1 Issue related to Tinc 1.1 enhancement New feature requests or performance improvement. merge_conflict labels Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.1 Issue related to Tinc 1.1 enhancement New feature requests or performance improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants